test: clarify coinbase sigop accounting in GetTransactionSigOpCost#92
Draft
test: clarify coinbase sigop accounting in GetTransactionSigOpCost#92
GetTransactionSigOpCost#92Conversation
l0rinc
changed the title
GetTransactionSigOpCost
The previous monolithic test mixed several independent vectors, so the first failure would stop coverage of the remaining ones. Split the vectors into focused test cases to make failures easier to diagnose and keep follow-up edits reviewable. It's best to view this commit without whitespace changes.
`assert()` aborts the entire test run on the first failure, hiding which other vectors are broken and providing little context. Switch these checks to Boost assertions and reuse a shared set of standard script verification flags.
`GetTransactionSigOpCost` treats coinbase transactions specially: they only contribute legacy sigops, skipping P2SH and witness accounting and avoiding input lookups. Add explicit coverage for coinbase-shaped inputs in both witness and P2SH contexts, and assert the witness precondition so the intent stays clear. Co-authored-by: Gary Krause <gary.krause@mara.com>
l0rinc
force-pushed
the
l0rinc/test-coinbase-sigopcost
branch
from
d46bc6a to
be4e602
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a reimplementation of the abandoned bitcoin#32840.
Context
This PR makes the
GetTransactionSigOpCosttest coverage explicitly document (and guard) the coinbase special-case: coinbase transactions only contribute legacy sigops and skip P2SH and witness sigop accounting.The existing GetTxSigOpCost test was monolithic and didn’t make the coinbase behavior obvious, which can lead to confusion (e.g. when reasoning about sigop budgets in mining/Stratum-related work) and makes regressions harder to spot.
Changes